Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for setting target path in map config #4694

Merged

Conversation

jgoakley
Copy link
Contributor

@jgoakley jgoakley commented Nov 10, 2023

Proposed change

Type of change

This change is to add support for configuring the target path for custom addons. In order to persist data outside of the addon container certain paths need to be bound. Currently there are several options to bind path into an addon (data, config, share, etc) but the paths inside the container are set.

Some applications and existing docker images have default storage paths that are not easily changed (if at all). Being able to configre these paths will allow simple integration with these applications. I have created a feature request in the Home Assistant Community Forum that provides some more detail:

https://community.home-assistant.io/t/ability-to-configure-container-target-path-for-custom-addon-such-as-wordpress/635615

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (which adds functionality to the supervisor)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast supervisor tests)
  • Tests have been added to verify that the new code works.

If API endpoints of add-on configuration are added/changed:

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jgoakley

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@jgoakley
Copy link
Contributor Author

jgoakley commented Nov 10, 2023

Hi!
This is my first time creating a pull request for home assistant. I am not sure how to run local tests for the supervisor app, but if that is something that is required for me to do I would be glad to do so! I might need some help in that area though.

If there is anything else I can help with or questions I can answer please feel free to let me know!

Thanks!

@jgoakley jgoakley marked this pull request as ready for review November 10, 2023 23:19
@pvizeli pvizeli added the new-feature A new feature label Nov 13, 2023
supervisor/addons/validate.py Show resolved Hide resolved
supervisor/addons/model.py Outdated Show resolved Hide resolved
@pvizeli pvizeli marked this pull request as draft November 13, 2023 17:32
@jgoakley
Copy link
Contributor Author

I added the FolderMapping dataclass and made the corresponding changes. I tried for the last couple of hours to get a local supervisor running against the main branch with no success. Here is the error message in case it is helpful:

ERROR: failed to solve: process "/bin/ash -o pipefail -c set -x && apk add --no-cache coreutils eudev eudev-libs git libffi libpulse musl openssl yaml && curl -Lso /usr/bin/cosign \"https://github.com/home-assistant/cosign/releases/download/${COSIGN_VERSION}/cosign_${BUILD_ARCH}\" && chmod a+x /usr/bin/cosign" did not complete successfully: exit code: 6

Anyway, the reason I say that is because there may be issues that I'm not able to test so if you encounter any issues with my changes please let me know and I will do my best to resolve them.

Thanks!

@jgoakley jgoakley marked this pull request as ready for review November 16, 2023 02:42
@pvizeli
Copy link
Member

pvizeli commented Nov 16, 2023

I added the FolderMapping dataclass and made the corresponding changes. I tried for the last couple of hours to get a local supervisor running against the main branch with no success. Here is the error message in case it is helpful:

ERROR: failed to solve: process "/bin/ash -o pipefail -c set -x && apk add --no-cache coreutils eudev eudev-libs git libffi libpulse musl openssl yaml && curl -Lso /usr/bin/cosign \"https://github.com/home-assistant/cosign/releases/download/${COSIGN_VERSION}/cosign_${BUILD_ARCH}\" && chmod a+x /usr/bin/cosign" did not complete successfully: exit code: 6

Anyway, the reason I say that is because there may be issues that I'm not able to test so if you encounter any issues with my changes please let me know and I will do my best to resolve them.

Thanks!

You need a new version of devcontainer -> https://github.com/home-assistant/devcontainer

Copy link
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

supervisor/const.py Outdated Show resolved Hide resolved
supervisor/addons/configuration.py Show resolved Hide resolved
@pvizeli pvizeli marked this pull request as draft November 16, 2023 08:17
Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jeff-oakley-lcs

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @jeff-oakley-lcs

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Copy link
Contributor

@mdegat01 mdegat01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once #4760 goes this can be rebased off that and then the dataclass moved to addons/data.py as per pvizeli's suggestion. Looks like its all ready to go at that point.

@mdegat01
Copy link
Contributor

Oh shoot. @pvizeli we can't put this in addons/data.py as that is already taken by something entirely different. The class for managing config data of an addon is in there and so yea we now get a circular import issue trying to use it for common dataclasses.

How about addons/configuration.py like we do in resolution instead?

@jgoakley
Copy link
Contributor Author

Oh shoot. @pvizeli we can't put this in addons/data.py as that is already taken by something entirely different. The class for managing config data of an addon is in there and so yea we now get a circular import issue trying to use it for common dataclasses.

How about addons/configuration.py like we do in resolution instead?

@mdegat01 I moved the FolderMapping dataclass to configuration.py as you suggested. If there are any other changes I should make just let me know!

@jgoakley jgoakley marked this pull request as ready for review December 14, 2023 18:47
@home-assistant home-assistant bot requested a review from mdegat01 December 14, 2023 18:47
@mdegat01
Copy link
Contributor

mdegat01 commented Dec 21, 2023

@jgoakley it looks like there's conflicts with the base branch. Can you rebase and resolve that? Then this is good to get merged.

@jgoakley
Copy link
Contributor Author

jgoakley commented Dec 22, 2023

@jgoakley it looks like there's conflicts with the base branch. Can you rebase and resolve that? Then this is good to get merged.

Hi @mdegat01, I think I accidently did a merge commit instead of a rebase when clicked "update branch" in GitHub. Sorry about that, I'm not very familiar with rebasing. I also synced my fork and performed a merge in VS Code but I didn't see any conflicts. Is "main" the correct base branch I should be merging into?

@agners agners dismissed home-assistant[bot]’s stale review December 27, 2023 15:16

Has been signed meanwhile

@mdegat01
Copy link
Contributor

@jgoakley all good on the merge vs. rebase. We prefer rebases since it makes the PR easier to review as the only commits are your changes, not the merges. But we squash and merge in the end so it doesn't matter after its merged.

Thanks for the contribution 👍

@mdegat01 mdegat01 merged commit e08c8ca into home-assistant:main Dec 27, 2023
23 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants